-
Notifications
You must be signed in to change notification settings - Fork 624
Build XNNPACK as an ExternalProject #12425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Stack from ghstack (oldest at bottom): |
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12425
Note: Links to docs will display an error until the docs builds have been completed. ❌ 114 New Failures, 1 Cancelled Job, 3 Unrelated FailuresAs of commit 466f669 with merge base b9bb3c1 ( NEW FAILURES - The following jobs have failed:
CANCELLED JOB - The following job was cancelled. Please retry:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
[ghstack-poisoned]
Apple build is failing because it can't find libXNNPACK.a: https://github.com/pytorch/executorch/actions/runs/16251337270/job/45881437516?pr=12425#step:9:47386 android build is failing because I didn't propagate cross compilation settings to the ExternalProject: https://github.com/pytorch/executorch/actions/runs/16251337288/job/45881440178?pr=12425#step:15:16927 llama runner linux build is failing because CMake is passing -lXNNPACK instead of the path to the static lib. (this happened during debugging before and it was because the XNNPACK target wasn't defined; probably the same again?) https://github.com/pytorch/executorch/actions/runs/16251337288/job/45881439862?pr=12425 unittest failures don't look related but we'll want to keep an eye on them. |
I want to see EXPORT work locally before I come back and fix broader platform support, so I'm going to do that first. Converting to draft in the meantime. |
build_frameworks_ios error:
|
test-llava-runner-linux failure looks like a flake: https://github.com/pytorch/executorch/actions/runs/16301129189/job/46035741014?pr=12425#step:15:15293 . therefore publishing for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes in the XNNPACK cmake/dependencies.cmake look good to me
include(cmake/Dependencies.cmake) | ||
set(xnnpack_third_party XNNPACK pthreadpool extension_threadpool cpuinfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious what does this rearrangment do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found it weird that we were mutating xnnpack_third_party in cmake/Dependencies.cmake rather than setting it all at once
set_target_properties( | ||
XNNPACK PROPERTIES INTERFACE_LINK_LIBRARIES xnnpack-microkernels-prod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh shoot does this mean that any targets that link XNNPACK will also automatically link xnnpack-microkernels-prod as well? So we don't have to explicitly list both XNNPACK and xnnpack-microkernels-prod? I was trying to figure out how to do this for a while, but never figured it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target_link_libraries with visibility PUBLIC or INTERFACE does this for normal libraries, but you need to do it this way for IMPORTED libraries.
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}) | ||
add_library(kleidiai SHARED IMPORTED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think i merged something that might have conflict here. I just added a check
if (TARGET kleidiai)
before adding the library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be fine I think
bunch of flakes that need to be rerun to make sure but no actual problems in the 7 failures so far |
Apparently FetchContent is preferred over ExternalProject. The thing I wanted from ExternalProject is separating our build from the dependency's build (i.e., not using add_subdirectory), because the XNNPACK dependency ecosystem doesn't support EXPORT. IIUC, we can get that separation using FetchContent by configuring it to support find_package (using either |
I believe this is necessary to support EXPORT. (We probably should not
land this unless we have a PR stacked on top of this that successfully
converts the build to EXPORT; sending this out so I can get CI going)